Conversation
jareth-whitney
left a comment
jareth-whitney
left a comment
There was a problem hiding this comment.
Great job streamlining our link system. It will be much easier to update our links in one place than to update all the individual links. Nothing blocking, just a few things to check out:
- Consider validating the route params in your getPath() function to avoid value ?? ''
- Awaits without suspense: probably fine, just a different pattern than what we've used
- Path concat questions (is forward slash in the middle?)
- Weird translation concat for prop (probably fine, just a weird translation implementation)
- language vs language: language
| <Grid size={12} mt={5}> | ||
| <FormProvider {...engagementCreationForm}> | ||
| <EngagementForm isNewEngagement onSubmit={onSubmit} /> | ||
| <EngagementForm engagement={null} onSubmit={onSubmit} /> |
There was a problem hiding this comment.
Nice, one less unneeded prop.
|
|
||
| const getTargetPreviewBasePath = () => `${getBasePathPrefix()}/engagements/${engagementId}/preview`; | ||
| const getTargetPreviewBasePath = () => | ||
| `${getBasePathPrefix()}/${getPath(ROUTES.ADMIN_ENGAGEMENT_PREVIEW, { engagementId: engagementId ?? '' })}`; |
There was a problem hiding this comment.
You could simplify this line a bit if you validate the engagementId (or any other route params) within your getPath() function. Then you just need to use { engagementId } for the second arg.
| style={{ position: 'relative', bottom: '4px' }} | ||
| /> | ||
| </IconButton> | ||
| <Await resolve={slug}> |
There was a problem hiding this comment.
No suspense needed here?
| const baseSurveyStatus = getStatusFromStatusId(loadedEngagement.submission_status); | ||
| const [currentPanel, setCurrentPanel] = React.useState('email'); | ||
| const [isEmailModalOpen, setIsEmailModalOpen] = React.useState(false); | ||
| const languageId = sessionStorage.getItem('languageId') ?? language ?? AppConfig.language.defaultLanguageId; |
There was a problem hiding this comment.
I couldn't find a definition for sessionStorage and then discovered that it's equivalent to window.sessionStorage and is best practice, so all good!
| // React Router context and navigate to a new base URL to pass the new | ||
| // tenant in the path and trigger a full reload of the app with the | ||
| // new tenant's context | ||
| href={`/${tenant.short_name}${getPath(ROUTES.HOME)}`} |
There was a problem hiding this comment.
Just checking if the forward slash is automatically placed between tenant short name and the home route. I'm guessing it is automatically generated in getPath().
There was a problem hiding this comment.
Yes; getPath() returns a leading slash
| </Grid> | ||
| <Grid sx={{ marginLeft: 'auto', marginRight: '32px' }}> | ||
| <Link onClick={UserService.doLogout} to={'#'}> | ||
| <Link onClick={UserService.doLogout} href={'#'}> |
There was a problem hiding this comment.
Yes, this is the best way to do a self-link.
| data-testid="link-container" | ||
| > | ||
| {translate('dashboard.link.0') + engagement.name + translate('dashboard.link.1')} | ||
| {translate('dashboard.link.0')} {engagement.name} {translate('dashboard.link.1')} |
There was a problem hiding this comment.
This may require some explanation. It looks like we're constructing a link but the translation integration is a bit weird.
| navigate( | ||
| getPath(ROUTES.PUBLIC_ENGAGEMENT_BY_SLUG, { | ||
| slug: slug.slug, | ||
| language: language, |
There was a problem hiding this comment.
You can just use language here, no need for language: language.
| navigate( | ||
| getPath(ROUTES.PUBLIC_ENGAGEMENT_BY_SLUG, { | ||
| slug: slug.slug, | ||
| language: language, |
There was a problem hiding this comment.
Same thing here: language over language: language
| export const getSlugByEngagementId = async (engagementId: number): Promise<GetSlugByEngagementIdResponse> => { | ||
| if (!engagementId || isNaN(Number(engagementId))) { | ||
| return Promise.reject('Invalid Slug' + engagementId); | ||
| return Promise.reject('Invalid ID' + engagementId); |
There was a problem hiding this comment.
Does this need a space for the concat?
|
3 participants
Issue #: 🎟️ DEP-255
Description of changes:
getPath()function fromroutes/routes.ts, which generates URLs based on a centralized ROUTES constant. This ensures that all links are consistent and automatically updated if route paths change in the future.User Guide update ticket (if applicable):
Common component changes: